Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tiled pixi tilemap rendering #1901

Closed
wants to merge 388 commits into from
Closed

Tiled pixi tilemap rendering #1901

wants to merge 388 commits into from

Conversation

blurymind
Copy link
Contributor

@blurymind blurymind commented Jul 31, 2020

This is a wip PR of the tilemap extension.
#503

image

write documentation (After merge)

Edit: the pixi5 regression has now been fixed and this PR is back on track!

@blurymind blurymind changed the title Tiled pixi tilemap - WIP Tiled pixi tilemap - WIP (not ready for review) Jul 31, 2020
@blurymind blurymind requested a review from 4ian as a code owner August 12, 2020 17:15
@Bouh
Copy link
Collaborator

Bouh commented Aug 17, 2020

Hi
I need to use Pixi v5.3.3, is there a problem for you @blurymind if I upgrade the version?
I need it to finish the BitmapText object, there are some important corrections.

@blurymind
Copy link
Contributor Author

Hi
I need to use Pixi v5.3.3, is there a problem for you @blurymind if I upgrade the version?
I need it to finish the BitmapText object, there are some important corrections.

Not a problem at all :) Currently the UMD module stuff breaks pixi-tilemap, which has nothing to do with pixi's version. Pixi-tilemap should work ok with the latest version

@4ian
Copy link
Owner

4ian commented Aug 20, 2020

@blurymind Is the UMD module still not working on pixi-tilemap (even outside GDevelop)?

@blurymind
Copy link
Contributor Author

blurymind commented Aug 20, 2020

@blurymind Is the UMD module still not working on pixi-tilemap (even outside GDevelop)?

Yes, positive. I tested it on a minimal pixi project.
Here is the actual PR:
pixijs-userland/tilemap#95
One of my comments has a link to the test project.

I closed my pr in favour of the more complete one there. Progress is on hold until it works as UMD.

I am trying to help the developer get it to work though and he's making progress.

@blurymind
Copy link
Contributor Author

blurymind commented Aug 20, 2020

A small update on this - the UMD module at the PR
pixijs-userland/tilemap#95
now works in the minimal pixi project.
It however still crashes in gdevelop with the same error message :(

I get this

TypeError: this.children[i].render is not a function
Container.render
D:/DEV/GD/GDevelop/newIDE/src/Container.ts:550
Container.render
D:/DEV/GD/GDevelop/newIDE/src/Container.ts:550
Container.render
D:/DEV/GD/GDevelop/newIDE/src/Container.ts:550
Container.render
D:/DEV/GD/GDevelop/newIDE/src/Container.ts:550
Renderer.render
D:/DEV/GD/GDevelop/newIDE/src/Renderer.ts:404
InstancesEditor._renderScene
D:/DEV/GD/GDevelop/newIDE/app/src/InstancesEditor/index.js:735
  732 |     this.windowBorder.render();
  733 |     this.windowMask.render();
  734 |     this.statusBar.render();
> 735 |     this.pixiRenderer.render(this.pixiContainer);
      | ^  736 |   }
  737 |   this.nextFrame = requestAnimationFrame(this._renderScene);
  738 | };

OR

Uncaught TypeError: Cannot read property 'transform' of null
    at RectTileLayer.Container.updateTransform (Container.ts:428)
    at CompositeRectTileLayer.Container.getLocalBounds (Container.ts:506)
    at CompositeRectTileLayer.get (Container.ts:669)
    at RenderedTileMapInstance.getDefaultWidth (D:\DEV\GD\GDevelop\newIDE\app\resources\GDJS\Runtime\Extensions\TileMap\JsExtension.js:494)
    at LayerRenderer.getInstanceWidth (LayerRenderer.js:92)
    at LayerRenderer._isInstanceVisible (LayerRenderer.js:178)
    at InitialInstanceJSFunctor.LayerRenderer.instancesRenderer.invoke (LayerRenderer.js:59)
    at 8152 (libGD.js?cache-buster=5.0.0-beta98-e240a7c80d1abab373f93f826c8ca5fa39b63774:51)
    at _emscripten_asm_const_iii (libGD.js?cache-buster=5.0.0-beta98-e240a7c80d1abab373f93f826c8ca5fa39b63774:51)
    at Array.gC (libGD.js?cache-buster=5.0.0-beta98-e240a7c80d1abab373f93f826c8ca5fa39b63774:25)
    at Array.ib (libGD.js?cache-buster=5.0.0-beta98-e240a7c80d1abab373f93f826c8ca5fa39b63774:25)
    at pF (libGD.js?cache-buster=5.0.0-beta98-e240a7c80d1abab373f93f826c8ca5fa39b63774:25)
    at Is (libGD.js?cache-buster=5.0.0-beta98-e240a7c80d1abab373f93f826c8ca5fa39b63774:25)
    at InitialInstancesContainer.IterateOverInstancesWithZOrdering.InitialInstancesContainer.IterateOverInstancesWithZOrdering [as iterateOverInstancesWithZOrdering] (libGD.js?cache-buster=5.0.0-beta98-e240a7c80d1abab373f93f826c8ca5fa39b63774:51)
    at LayerRenderer.render (LayerRenderer.js:204)
    at InstancesRenderer.render (index.js:117)
    at InstancesEditor._renderScene (index.js:728)

@4ian
Copy link
Owner

4ian commented Aug 20, 2020

Ah great, at least a proper UMD module :) So it seems that there is something that is null/undefined being passed down to PIXI.

If we look at Container.ts:428, we see this:
https://github.com/pixijs/pixi.js/blob/8f7ed7662949ea1217b6f3b6af470e855cfe0b13/packages/display/src/Container.ts#L428
seems like this.parent is undefined, which is weird. Are we passing something bad to Pixi at some point when adding a child?

@blurymind
Copy link
Contributor Author

blurymind commented Aug 20, 2020

Ah wait, I think I celebrated too early - his PR has the umd module built with the umd name, but inside it, do you see any wrapping?
https://github.com/SukantPal/pixi-tilemap/blob/4d395e6566f917d031b85aeb62ffa0d295b75d2c/dist/pixi-tilemap.umd.js

Its not wrapping everything, just some functions.

but still works fine in the minimal project outside of Gdevelop

@4ian
Copy link
Owner

4ian commented Aug 20, 2020

Weird, seems not like a real UMD module, it will work without issue in the browser, but I see no "require" at all that would make it compatible with "CommonJS" and so it's possibly not loaded properly in Electron.

@blurymind
Copy link
Contributor Author

@4ian thank you, I opened a new ticket here
pixijs-userland/tilemap#97

@4ian
Copy link
Owner

4ian commented Aug 21, 2020

Can you give me a tiled json and tilemap atlas image so I can quick check something?

@blurymind
Copy link
Contributor Author

Can you give me a tiled json and tilemap atlas image so I can quick check something?

It's included in the PR :)

the atlas is:
Extensions/TileMap/example/Viking3.png

the tiledMap json is
Extensions/TileMap/example/island.json

Sorry, I should rename them for clarity.
I also included the original tmx file, just in case

@4ian
Copy link
Owner

4ian commented Aug 21, 2020

The PIXI object when logged has a "render" method that was overriden by something to be now "visible":
image

This is the cause of the crash. I'm not sure why this is happening. It seems that something was mixed up between the GDevelop instance and the pixi object?

@4ian
Copy link
Owner

4ian commented Aug 21, 2020

This 😬

image

@blurymind
Copy link
Contributor Author

blurymind commented Aug 21, 2020

good catch.I should probably rename this property to get around the problem? :)
It controls whether to render a specific layer or all layers.

I am thinking of expanding it to also be able to only render a specified group

Maybe call it displayMode?

@blurymind
Copy link
Contributor Author

I updated the name of the property now.
We still have the other error with Uncaught TypeError: Cannot read property 'transform' of null.

I suspect that is due to the UMD module not working properly yet?

@4ian
Copy link
Owner

4ian commented Aug 21, 2020

For Cannot read property 'transform' of null :

I added a log in PIXI and can see that there is a parent set to null on a RectTileLayer:
image

I'm not sure at all why this could be the case. Something to dig in Pixi Tilemap? Maybe we misused an API from Pixi Tilemap?

For render and other properties:

Why storing it inside the _pixiObject? The pixiObject is a CompositeRectTileLayer. We should never add new properties to it (because exactly of this issue: sooner or later you'll override a property used by PIXI). CompositeRectTileLayer is what it is, let's not modify it.

If you want to access to "render", just do this._associatedObject.getProperties(this.project).get('render').getValue(). But no need to store it right? Same for layerIndex? I see a this._pixiObject.layerIndex = layerIndex;, but this is never used, and same for tiledFile and tilemapAtlasImage

Let's clean these properties that are not used. Rule of thumb: if it's not part of PIXI Tilemap, don't set it :)

@4ian
Copy link
Owner

4ian commented Aug 21, 2020

I'm looking at Uncaught TypeError: Cannot read property 'transform' of null but I'm not sure what's happening 🤔 It worked previously right?

@blurymind
Copy link
Contributor Author

blurymind commented Aug 21, 2020 via email

@4ian
Copy link
Owner

4ian commented Aug 24, 2020

My belief is that its either due to pixi5, the umd or something I did when
merging the new updates from gdevelop into the branch. I wonder if it will
work in the web app. I only tested with electron

Yeah not sure what broke but this is very strange... maybe we could try to make it work first in games in the browser, to unblock this?
I've not tested in the web-app either, might be worth taking a look.

I'll be back to this to try to understand what is going wrong but it's quite time consuming so I have to alternate with other PR/issues :) I'll give this a new try though.

@blurymind
Copy link
Contributor Author

blurymind commented Aug 29, 2020

I tried with the updated pixi-tilemap library, I get this:

D:\DEV\GD\GDevelop\newIDE\app\resources\GDJS\Runtime\Extensions\TileMap\JsExtension.js:366 Uncaught TypeError: Cannot read property 'CompositeRectTileLayer' of undefined
    at new RenderedTileMapInstance (D:\DEV\GD\GDevelop\newIDE\app\resources\GDJS\Runtime\Extensions\TileMap\JsExtension.js:366)
    at Object.createNewInstanceRenderer (ObjectsRenderingService.js:62)
    at LayerRenderer.getRendererOfInstance (LayerRenderer.js:118)
    at InitialInstanceJSFunctor.LayerRenderer.instancesRenderer.invoke (LayerRenderer.js:51)
    at 8240 (libGD.js?cache-buster=5.0.0-beta98-9de9057d9c8201daf5436add5be4c942c51f1722:51)
    at _emscripten_asm_const_iii (libGD.js?cache-buster=5.0.0-beta98-9de9057d9c8201daf5436add5be4c942c51f1722:51)
    at Array.WC (libGD.js?cache-buster=5.0.0-beta98-9de9057d9c8201daf5436add5be4c942c51f1722:21)
    at Array.ib (libGD.js?cache-buster=5.0.0-beta98-9de9057d9c8201daf5436add5be4c942c51f1722:21)
    at dG (libGD.js?cache-buster=5.0.0-beta98-9de9057d9c8201daf5436add5be4c942c51f1722:21)
    at lt (libGD.js?cache-buster=5.0.0-beta98-9de9057d9c8201daf5436add5be4c942c51f1722:21)
    at InitialInstancesContainer.IterateOverInstancesWithZOrdering.InitialInstancesContainer.IterateOverInstancesWithZOrdering [as iterateOverInstancesWithZOrdering] (libGD.js?cache-buster=5.0.0-beta98-9de9057d9c8201daf5436add5be4c942c51f1722:51)
    at LayerRenderer.render (LayerRenderer.js:204)
    at InstancesRenderer.render (index.js:117)
    at InstancesEditor._renderScene (index.js:728)
...

@blurymind
Copy link
Contributor Author

blurymind commented Aug 29, 2020

actually when loading,there are now new errors
Capture
what do we need to change to allow these new requires?

@4ian
Copy link
Owner

4ian commented Aug 29, 2020

Thanks for trying again. I've pushed to master this commit: 0ac2ef7 which adds support for @pixi/... requires.

Can you try one more time with this? We should be "back to" the issue of the transform being null (Cannot read property 'transform' of null), which might be because of a Pixi v5 update/issue or something like that?

@4ian
Copy link
Owner

4ian commented Aug 29, 2020

Actually no need to merge master, I've updated the file in this branch :)

@4ian
Copy link
Owner

4ian commented Aug 29, 2020

The pixi tilemap UMD file is still not working 😬😬 Because there is a footer that was added, I'm removing it now.

@blurymind
Copy link
Contributor Author

Quick question, I have this tileset json file made in Tiled:
TestTileSet.json.zip

Can you include the image file as well as the tilemap file? All the files that are intended to be used in GD

@4ian
Copy link
Owner

4ian commented Dec 7, 2020

Sure, here you go:
test tilemap.zip

I'm sorry files were in different directories so if you want to try to open it in Tiled you might have to change these in the JSON directly?
What I did was:

  • open tiled, make a new map
  • choose sheet.png for the tileset, choose somewhere where to save it
  • Draw something, then save. I also tried to do a File > export as...
    image

@4ian
Copy link
Owner

4ian commented Dec 7, 2020

Somehow I cannot select a tilemap in the scene editor, nor move it. I have the impression this bug was already there since I started working on this, but let me know if that's not the case and I broke something?

@blurymind
Copy link
Contributor Author

Somehow I cannot select a tilemap in the scene editor, nor move it. I have the impression this bug was already there since I started working on this, but let me know if that's not the case and I broke something?

I noticed that you can only box select tilemaps. My theory is that gdevelops ide requires us to set something on the pixi object to make it click selectable 🤔

@4ian
Copy link
Owner

4ian commented Dec 8, 2020

Indeed the tilemap is not "interactive" by default as containsPoint is not implemented. I'll add this :)

@4ian
Copy link
Owner

4ian commented Dec 8, 2020

This is implemented, selection/move/resize in the scene editor works well.
A few things to check (update of the instance renderer), a few more refactoring to pixi-tilemap-helper to use maybe promises, or at least always call the callbacks (actually they were not useful), and we should be good!

// Implement `containsPoint` so that we can set `interactive` to true and
// the Tilemap will properly emit events when hovered/clicked.
// By default, this is not implemented in pixi-tilemap.
this._pixiObject.containsPoint = (position) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps @ivanpopelyshev might be interested in having it added to pixi-tilemap itself? Or I suppose there is a good reason for not having any extra stuff like this

@Silver-Streak
Copy link
Collaborator

@blurymind As this seems like it's getting really close to implementation, and finances are a bit more stable, I've bumped up the bounty on this again. Total is now at $175.

@blurymind
Copy link
Contributor Author

blurymind commented Dec 9, 2020

@blurymind As this seems like it's getting really close to implementation, and finances are a bit more stable, I've bumped up the bounty on this again. Total is now at $175.

@Silver-Streak I humbly thank you for the generous donations 🙇
I also want to thank everyone else involved in making this happen. I will test the problematic file @4ian shared and see if I can fix it in my helper module.
There is also more to be done on the documentation, so I will try to add more info by the end of the week

@4ian
Copy link
Owner

4ian commented Dec 10, 2020

@blurymind Thanks! For the documentation, remember to write it in the same style as other objects, for example: http://wiki.compilgames.net/doku.php/gdevelop5/objects/tiled_sprite

Notably, it's important to remember that the audience is the user of GDevelop, so we avoid talking about technical details:

  • don't talk about Tilemap "extension", it's an object.
  • Avoid talking about pixi-tilemap - it's an "implementation detail". It's a good idea to have a paragraph about limitations, but we now "own" these limitations, i.e: these limitations are now limitations of the GDevelop object itself. So we avoid telling to the user "well it's limited because of this library".
  • In other words, the lambda user don't care about Pixi, pixi tilemap or whatever. They just want to know:
    1. How to use this object (what should I do, download Tiled, and then?)
    2. If there are limitations.
    3. Any advice about how to use this object (the paragraph on the animation could be a dedicated section for example).

Also some nitpickings ;)

  • Avoid all hand drawn arrows/shapes/numbers. Prefer to use anything else: Google Slides, Powerpoint, even Paint but nothing hand drawn, as it looks not super professional:
    image
  • Careful about the casing of GDevelop (not Gdevelop) (I'm nitpicking, but still such small details can make the difference between looking pro or subpar to power users and the outside world).
  • Proof read with something like Grammarly for spotting some typos.

I'll proofread and edit this anyway before we distribute the object publicly, so if you're not sure go ahead and I'll make fixes :)
I still have to do a few last fixes that I listed before on the code itself - and figure out too what's wrong with the tilemap/tileset I made :)

Almost there! ;)

@4ian 4ian added the 🏁PR almost ready: final fixes The PR is almost ready and needs final fixes and/or polishing before merge label Dec 10, 2020
@blurymind
Copy link
Contributor Author

at least the wiki should make it clearer that you need to disable it by default and reenable it to give the user a choice. Right now, it seems to say "use the action at the beginning of the scene to permanently disabled analytics". It probably isn't meant that way but if someone (me) interpr

hmm are you sure you commented in the right PR? :)

@blurymind
Copy link
Contributor Author

blurymind commented Dec 11, 2020

@4ian why are there suddenly over 900 files in the dif? :) you must have pushed a big change.
I wanted to investigate the example tiled files you attached, but I cant seem to pull changes anymore. Had to clone the branch afresh

I will try to update the wiki in the weekend. You are right, that graphic is lazy

@@ -38,6 +38,17 @@ void GD_CORE_API BuiltinExtensionsImplementer::ImplementsWindowExtension(
true)
.SetDefaultValue("yes");

extension
.AddCondition(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like some commit from another pr somehow bleeded over this one

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a GitHub issue, try to close this PR and open another one. That should do the trick. It's because master got merged in this PR.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just checking in on this, is the new PR going to be a big issue for progress, @blurymind ?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally it's nothing at all, should just give more clarity before merging.

@4ian
Copy link
Owner

4ian commented Dec 20, 2020

@blurymind Could you see if you can close this PR and open another one (don't do any change, just close the PR and open another one from tiled-pixi-tilemap branch)? This should properly show the changed files, so I can polish the last things. :)

We should also have explanations on the wiki about how to use Tiled - even if it's very succinct (I'm happy to proofread/rewrite part of the page).

@blurymind
Copy link
Contributor Author

blurymind commented Dec 21, 2020

Ok I will close this one and open a new one. Sorry for my slow reactions atm. I was asked to self isolate (again) in a place with no internet connection so its a bit hard to work with my mobile broadband. Hopefully this will be over in a week or so.

The airports are closed to the uk so it might become impossible to get back home in the next month, but at least I will get a normal internet connection again and be able pull gigabites of node modules to rebuild GDevelop and see whats going on with that file. In the time being it shouldnt be hard to at least update the documentation

@blurymind blurymind closed this Dec 21, 2020
@blurymind blurymind mentioned this pull request Dec 21, 2020
21 tasks
@blurymind
Copy link
Contributor Author

I was able to recreate the PR at #2147

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏁PR almost ready: final fixes The PR is almost ready and needs final fixes and/or polishing before merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Tilemaps objects (tilesets and Tiled files)